I'm trying to understand the purpose of the mount("/", "/", NULL, MS_REC | MS_PRIVATE, NULL) in the first place. Why is it necessary? The tmpfs mounts that we do afterwards don't seem to require that. I'm pretty sure there's a reason for it, just not seeing it right now 😄

That's a good question. I basically copy-pasted it from #2617, assuming this is just part of the magic incantation to make any mount changes in the scriptlet private to the scriptlet. But, isn't that what CLONE_NEWNS is about? Looking at mount(2):

   A child process created by fork(2) shares its parent's mount namespace;
   the mount namespace is preserved across an execve(2).

   A process can obtain a private mount namespace if: it was created using
   the  clone(2) CLONE_NEWNS flag, in which case its new namespace is ini‐
   tialized to be a copy of the  namespace  of  the  process  that  called
   clone(2);  or  it  calls  unshare(2)  with  the CLONE_NEWNS flag, which
   causes the caller's mount namespace to obtain a  private  copy  of  the
   namespace  that it was previously sharing with other processes, so that
   future mounts and  unmounts  by  the  caller  are  invisible  to  other
   processes (except child processes that the caller subsequently creates)
   and vice versa.

@jsegitz, do you remember what the deal with the / mount was?

Yep, my thoughts as well, we are unsharing a namespace already, and it's a mount namespace because of the CLONE_NEWNS flag (as described in the man page). I guess the mount call must have had a different purpose then.

Looking at the flags used in the call:

MS_REC (since Linux 2.4.11)
  Used in conjunction with MS_BIND to create a recursive bind  mount,  and  in  conjunction
  with  the propagation type flags to recursively change the propagation type of all of the
  mounts in a subtree.  See below for further details.
[...]
MS_PRIVATE
  Make  this  mount private.  Mount and unmount events do not propagate into or out of this
  mount.

So my suspicion is that this has to do with preventing (some kind of) mount propagation but I'm yet to understand this topic better. LWN to the rescue: https://lwn.net/Articles/690679/

One potentially relevant difference between the original PR and the plugin is that the plugin actions take place in the already forked child process, whereas the original worked in the main rpm process.

At any rate, this PR looks more and more like the wrong thing to do.

At any rate, this PR looks more and more like the wrong thing to do.

Indeed, although just disabling the plugin in chroots (i.e. what this patch effectively does) sounds like a good-enough workaround until we have a proper solution/understanding.

My takeaway from some further poking + studying is that it's more complicated than we want to handle just now. Made the disabled in chroots -behavior explicit both in the docs and a warning message, I think that's the best we can do for 4.20. But instead of closing the ticket, we'll just move it back to the backlog with a bunch of added info.

Sorry for the delay, I was away on a longer holiday (well, not sorry actually ;)).

The reason is the one mentioned by @dmnks. I was also surprised by this, because my assumption was that a mount namespace is self contained. But withou this explicit mount call I saw side effects outside of the namespace.

I'm not going to pretend that I fully understand this ...

Right, I had similar experiences with this, without the explicit private mount the namespace doesn't seem to mean much because the mounts still propagate outside somehow. It's a weird thing.

I think I saw something in the lines of "developers soon found the private mount namespace is in fact too private and had to change it" when looking for info on this.

OK, I think I have a better understanding of this mount propagation thingy now (and still improving, as the history of edits in this comment would suggest 😄).

First of all, the "remount" of / we do in the unshare plugin isn't really a remount, it merely changes the propagation type of an existing mount. In fact, the source argument is ignored by mount() in this case. We should probably replace it with NULL to make it less confusing.

What the call does here is that it recursively changes the propagation type of / to private (MS_REC | MS_PRIVATE). This is needed because, by default (on Linux with systemd at least), / defaults to shared, meaning that any mount events occurring in the new namespace will propagate to the parent namespace (i.e. the host) and vice-versa. That's not what we want, obviously, we want full isolation, hence the call.

Much like the unshare command which assumes the user indeed wants full isolation by default, so it performs the equivalent of our mount() call upon entering the new namespace. In the shell, the equivalent is mount --make-rprivate /. You can actually disable this step by passing --propagate unchanged to unshare.

In fact, I've tried this with the unshare plugin by buildling a simple package with a %post scriptlet that does a bind mount, and indeed, with the mount() call commented out, mounts did propagate between the scriptlet's namespace and the host in both directions.

So that explains the "mysterious" mount() call. The thematic LWN article is a good read that summarizes all this.

Now, the reason it fails in a chroot is simple, it tries to change the propagation type on the chrooted directory which (likely) is not a mount point. This is basically what @pmatilai realized in the original ticket #3187 already.

The core issue here is that unshare & chroot are two competing operations, in a sense. I think, in long term, it would be better to make the unshare plugin somehow "replace" the chroot functionality altogether, instead of trying to work around it. That, or have a plugin hook in RPM that runs before chroot, which the unshare plugin would then use instead and thus run the mount() call before chroot. But that's beyond the scope in this PR, of course, just noting it here for future reference.

Besides the independent notes above, changed the mount propagation call source to NULL and changed the error message more specific for that case. Also, rebased.

And thanks for the in-depth research, I ran out of patience around 1/4 of the way 😅

Thanks, LGTM now, merging.